-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix ComboBox
options' tag flicking
#2120
Conversation
i.e. Updating range based on next visibleCount/isOverflowing instead of current
Can be optimized a bit more.
…to avoid guessing what we already know underflows.
…when showing all items and still no overflowing
…d-options-jittering
…will need to remove containerRef as that's causing the effect to run twice
…but could investigate why two guesses happen with no change. i.e. double when called the first time doesn't double the guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to split this into multiple stacked PRs? It's extremely difficult to understand/review it right now (unless you're looking for a quick LGTM)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there's some flakiness/unreliability in the e2e tests when running on the GitHub CI (action run).
Investigating why it keeps failing only in the CI and not locally despite having await
s with the Promise
returning expect
s.
|
||
if (isOverflowing) { | ||
// overflowing = we guessed too high. So, new max guess = half the current guess | ||
newVisibleCountGuessRange = [visibleCountGuessRange[0], visibleCount]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that I understand how this part of the algorithm works. This is apparently halving the visibleCountGuesssRange
, but I don't know how visibleCount
is half of the max for the guess range. I must be missing something, since I am able to test the overflow container out with the deploy preview and see that it is working.
Changes
Fixes #2112. As mentioned in #2112 (comment), I believe the flickering is because of an infinite loop:
I think this is because our
useOverflow
hook makes the assumption that all items are around the same size and thus we can take an average:iTwinUI/packages/itwinui-react/src/utils/hooks/useOverflow.tsx
Line 79 in 4f431e5
While this may work for cases like the TablePaginator, it doesn't seem to work for the ComboBox case where tags could have different widths.
This PR refactors
useOverflow
with a new algorithm that now takes the actual widths/heights into consideration.New algorithm for useOverflow
Logic:
visibleCount
. e.g.[0, 32]
(32 is an arbitrary choice).i.e. the max guess should always be
≥
the correctvisibleCount
.visibleCount
to themaxGuess
.guessVisibleCount()
(keep re-rendering but not painting):visibleCount
.Why this works:
log(n)
.While working on
useOverflow
, it seemed hard to get it to work by usingref
s and also not knowing the children. So, this PR creates a new private component calledOverflowContainer
that callsuseOverflow
internally.useOverflow
is this now no longer used by any other component.OverflowContainer:
children
,overflowTag
andoverflowTagPlacement
(e.g. overflow from start or end). It automatically places theoverflowTag
at theoverflowTagPlacement
.(visibleCount) => React.ReactNode
) as the child ofOverflowContainer
. Now what to render based be customized based on the visibleCount.itemLength
is required in this case.I also tested that this bug does not regress some related prior bug fixes. Examples:
Select
andComboBox
for this case.Testing
useOverflow
now renders multiple times, having unit tests to mock rendered sizes is now unreasonable. So, I replaced all failing unit tests related touseOverflow
with e2e tests.Before/After
Before:
Enregistrement.de.l.ecran.2024-06-28.a.13.28.29.mov
After:
Enregistrement.de.l.ecran.2024-06-28.a.13.25.54.mov
Docs